Repository navigation
repack: the container is verified against the GGUF it came from, under the name that says so (refs #1109) - #1234
Conversation
PR Reviewer Guide 🔍(Review updated until commit 5a8f5ab)Here are some key observations to aid the review process:
|
…against a converter's name (#1109) The comparison this issue asks for already landed inside #1169's tile check: `tile_order_check` runs `q4nx_diff._decode_pass`, which dequantizes the source GGUF (`ref = dequantize(t.data, t.tensor_type)`) and compares every decoded container tensor against it under BOTH layouts. So a wrong checkout is already refused (`matches NO layout`, rc=1) -- what was missing is the contract around it, and the hole that reopened after the refusal. * `--verify-source <dir> <model.gguf>` (and the reusable `verify_source()` it wraps, with `tile_order_check` kept as its old name): the measurement under the name #1109 asks for, usable on an existing or published directory. Exit 0 verified in the engine-1 order, 1 another order or none, 2 nothing comparable. The repack calls the same function after the converter and the config write, so the two cannot disagree. * the verdict now names the SOURCE tensor and its correlation: a wrong checkout reads `matches NO layout: closest is flm with worst max|err| 3.460e+00, corr +0.0325 on blk.14.attn_output.weight`, against an honest pack's `corr +0.9994` (the issue's numbers). The correlation is reported, not gated: no floor was measured on honest containers, and the gate stays the measured max|err| rule. * the refusal names the converter checkout and commit the stamp is about to record -- the one fact repack-stamp.txt has and the user needs when a converter is the wrong version. * `--skip-verify` is the escape the issue's comment asks for (the verify costs a GGUF pass, for the tight loops the payload builds run in). It stamps `tile_order=unverified` with `tile_check=skipped (--skip-verify)` -- never `engine-1`. * `--check-stamp` no longer says `stamp: current` for a directory the repack REFUSED: the refusing path leaves model.q4nx on disk, so before this the next serve reused the container the guard had just rejected -- a wrong-converter container still served after being caught. A stamp that predates the measurement is stale (rebuilt, which is what makes it verified); a stamp that says `unverified` is "cannot tell" (2), so serve reuses it with its existing warning instead of silently, and does not re-run the failed measurement on every start. Deliberately NOT added: a second comparison path (the existing one compares against the source, not just against layouts), a correlation threshold (not measured on honest containers; a guess would refuse them), and attention-only scoping (padded MLP tensors have been readable since #1119/dd5fe38, so scoping would weaken the check the comment asked for). tests/repack_verify_source_test.py: a one-tensor synthetic GGUF, a one-tile container packed in each layout, and a fake converter that copies a prepared container, through the real entry points. It asserts the good container matches with corr +1.0000 and the tensor named, other values match nothing with corr near 0, an FLM-order container matches its source but is refused as this engine's order, `--verify-source` exits 0/1/2, `--skip-verify` stamps unverified and `--check-stamp` returns 2 for it, a real repack stamps `tile_order=engine-1` with the measured verdict, and a refused directory is stale on the next cache hit. Wired into ci.yml's python test step (numpy + gguf installed there). Run: copyright rc 0; the test above 0 failures; q4nx_diff --self-test, q4nx_diff_grid, repack_tile_order, tile_check_no_shared_tensor, npu_plan_tile_order, gguf_to_q4nx all pass.
…eds, and a pre-measurement stamp is rebuilt once (#1109)
6214f67 to
64050aa
Compare
|
Persistent review updated to latest commit 64050aa |
|
Head moved to via GitHub's update branch (main merged in, no content change): the branch had gone BEHIND within one build cycle. was green on (5m30s). Auto-merge is armed; the queue can land it as soon as this head's is green. |
|
Persistent review updated to latest commit e76d04f |
|
Persistent review updated to latest commit 5a8f5ab |
…a foreign-order directory is refused offline (Fixes #1110) (#1275) * npu: check_q4nx_dir --against measures the q4_1 TILE ORDER, so a foreign-order dir is refused offline engine#1110's surviving hole: the repack measures the tile order (#1169/#1234) and the plan loader refuses a declared order it does not implement (#1130), but a directory that was never repacked here -- imported from a foreign converter, downloaded, copied off a board -- has no stamp and no plan, and every metadata check passes: the two orders share the chunk kind, the byte count and the tensor names. Only the weights can say which order the bytes are in. --against already decoded the first Q8 tile both ways against the source GGUF; it now does the same for q4_1 over all three orders this repo can name -- ours (npu/q4nx.cpp's), flm signed, flm unsigned -- with tools/q4nx_diff.py's own decoders imported rather than re-implemented, and refuses a container whose winner is not ours: tensor name, all three correlations, and the closest foreign reading in the message. Accepted when ours >= 0.99 and ours - best_other >= 0.5, both measured: a container packed from Q4_K_M reads +1.0000, one packed from BF16 +0.996, every foreign order |corr| < 0.05. The metadata-only path still imports no numpy and no gguf. Tests: tests/check_q4nx_tile_order_test.py builds the case the issue describes -- one known 32x256 weight encoded as a single q4_1 tile in the engine's order and again in the foreign one, each encoder round-tripping through its own decoder first (4.17e-03) while the same bytes read as 'ours' are 1.31e-01 wrong -- then requires pass/refuse, the refusal to name the tensor, the foreign reading to be identified, and the verdict to survive a rename to another tensor. * npu: decide the q4_1 order on the margin, not a 0.99 proximity cut, and make the self-test follow the message Review blockers, both real. 1. `--self-test` exited rc 1 at every commit of this branch: the summary line was reworded ("no Q8 or q4_1 tile was decoded ...") and the assertion at the old line 522 still looked for "no Q8 tile was decoded". My own run had printed that failure and I quoted "self-test ok" from its tail without reading its exit code -- the exact claim-before-check mistake this lane keeps writing down. Fixed, and the self-test now also covers the q4_1 selector and the sampler (pure python: a 5120-B I8 tensor is q4_1, 4736/8704 are not; spread() reaches both ends). 2. `ours >= 0.99` was a proximity cut on quantization error, and it refused the PR's own non-regression row: on a full sweep of the published FastFlowLM Qwen3-0.6B-NPU2 directory the outlier-heavy `layers.2.mlp.down_proj` reads corr +0.9890 (rel-rms 0.1477) with a margin of ~0.95 over every foreign reading -- unmistakably the engine's order -- yet the check refused it and then said "NOT in the order, best is ours", which contradicts itself. The order is now decided on the MARGIN -- ours wins and beats the runner-up by >= 0.5 -- with a 0.9 floor that only rules out "no reading matches at all". Measured: correct containers lead by >= 0.95, foreign ones by <= 0.05 (the d1d5232 container: ours +0.0477 vs flm-unsigned +0.9961). The refusal now says which of the three things happened instead of one message for all: no reading matches (wrong GGUF or an unnameable order), a foreign order it can name, or an ambiguous margin (a partly foreign container). Also, per the review's non-blocking note: samples are spread evenly across the table instead of the first N in table order, so a foreign order that varies tensor to tensor is not missed by sampling one layer. Tests: tests/check_q4nx_tile_order_test.py grows to 14 checks. The new ones are the regression the floor caused (a correct container at corr +0.9827 must pass -- inside the band the old floor refused) and the wrong-source case (must say "no reading of these tiles matches", never claim a foreign order it cannot identify). Negative control: restoring `ours >= 0.99` makes exactly the new regression check fail and nothing else, so the test bites on this bug and not on a coincidence. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Refs #1109. Follows the issue's own last comment: the guard is "verify the container it just wrote against the GGUF it came from", not "name a table of converter commits".
What was already there (checked before writing anything)
#1169's tile check already compares the container against its source, not just against layouts:
tools/q4nx_diff.py:332dequantizes the GGUF tensor (ref = dequantize(t.data, t.tensor_type)) and:333compares every decoded container tensor with it, under both layouts, called fromrepack_gguf.pyafter the converter returns. So this PR adds no second comparison path. Measured on the issue: a wrong converter checkout readsmatches NO layoutwithcorr +0.0325where an honest pack readscorr +0.9994.What was missing, and is here
--verify-source <dir> <model.gguf>and the reusableverify_source()it wraps (tile_order_checkkept as the old name): the measurement under the name the issue asks for, usable on an existing or published directory. Exit 0 verified in the engine-1 order, 1 another order or none, 2 nothing comparable. The repack calls the same function, so the two cannot disagree.matches NO layout: closest is flm with worst max|err| 3.460e+00, corr +0.0325 on blk.14.attn_output.weight. Reported, not gated — no correlation floor was measured on honest containers, and the gate stays the measured max|err| rule so a guess cannot refuse a good pack.repack: converter: <dir> @ <commit>), the one factrepack-stamp.txthas and the person hitting it needs.--skip-verifyis the escape the issue's comment asks for (the verify costs a GGUF pass, which matters in the tight repack loops the payload builds run). It stampstile_order=unverifiedwithtile_check=skipped (--skip-verify)— neverengine-1.model.q4nxon disk, and--check-stampignoredrefused/tile_order: the next serve reportedstamp: currentand served the container the repack had just rejected. Now refused→stale (1), a stamp that predates the measuredtile_order→stale (rebuilt once, so it becomes verified), unverified→2 ("cannot tell": serve reuses it with its warning, and does not re-run the measurement that could not run).Tests
python3 tests/repack_verify_source_test.py— new, 0 failures, wired intoci.yml's python step (it needs numpy + gguf, which that step installs). It drives the real CLI with a one-tensor synthetic GGUF, one-tile containers packed in each layout, and a fake converter: good container matches withcorr +1.0000and the tensor named; other values match nothing with corr near 0; an FLM-order container matches its source but is refused as this engine's order;--verify-sourceexits 0/1/2;--skip-verifystamps unverified and--check-stampreturns 2 for it; a real repack stampstile_order=engine-1with the measured verdict; a refused directory is stale on the next cache hit.Also green:
tools/copyright.py --check,q4nx_diff.py --self-test,tests/q4nx_diff_grid_test.py,tests/repack_tile_order_test.py,tests/tile_check_no_shared_tensor_test.py,tests/npu_plan_tile_order_test.py,tests/gguf_to_q4nx_test.py.Not verified
No real GGUF/converter/model and no end-to-end
serve— this box is device-free for that path, as scoped. The synthetic arms exercise the same entry points.Reviewer notes
tile_order, so--check-stampnow returns 1 and serve rebuilds that cache dir once. A rebuild is the only way such a dir becomes verified. It only happens when the converter resolves (otherwise the answer is 2, fail open), so a box without its converter is not lost — but it is a one-time cost per cached dir on upgrade. Say if fail-open (2) is preferred for pre-measurement stamps.docs/npu.md(Lead-added in the second commit) now documents--verify-source,--skip-verifyand the rebuild-once rule.dd5fe38, so scoping would weaken the guard), and a correlation threshold.